Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add character countdown to commit message input #36890

Merged
merged 2 commits into from
Dec 12, 2017
Merged

Add character countdown to commit message input #36890

merged 2 commits into from
Dec 12, 2017

Conversation

jayjun
Copy link
Contributor

@jayjun jayjun commented Oct 25, 2017

See #21144.

I miss warnings against long commit messages. I like Atom’s way of showing a persistent character countdown. It’s subtle and easy to ignore.

31976595-0f44f22e-b96b-11e7-9519-5fbe554a2818

Knocked something up quickly using InputBox’s built-in validation box.

thvugwdgsf

Haven’t put much thought into the UI. Happy to hear more ideas!

@msftclas
Copy link

msftclas commented Oct 25, 2017

CLA assistant check
All CLA requirements met.

@jayjun jayjun changed the title [WIP] Add character counter to commit message input [WIP] Add character countdown to commit message input Oct 25, 2017
@scottclayton
Copy link

@jayjun

There is a setting in extensions/git/package.nls.json

"config.enableLongCommitWarning": "Whether long commit messages should be warned about",

I don't know how the settings work in VS Code presently, but you might be able to work with that setting about whether to display the box.

I would suggest adding an additional setting for the number to cut off rather than hardcoding 72… something like

"config.longCommitWarningThreshold": "The number representing the maximum limit for an acceptable commit message",

I don't have the build tools in front of me or anything yet.

As an aside: I try to keep all my commits to <= 50 characters, since I think that's what the cutoff used to be in VS Code before the feature was dropped this year :)

@jayjun
Copy link
Contributor Author

jayjun commented Oct 26, 2017

@scottclayton The warning feature was dropped when Git moved into an extension. config.enableLongCommitWarning exists as a vestige in package.nls.json only (localisation file).

I would imagine commit message limits are universal across SCMs anyway, within organisations or projects at least and not limited to the Git extension.

As for settings, I can’t find an IConfigurationService nearby to access values. Any ideas?

@joaomoreno joaomoreno self-requested a review October 27, 2017 09:58
@joaomoreno joaomoreno added this to the Backlog milestone Oct 27, 2017
@joaomoreno joaomoreno added git GIT issues scm General SCM compound issues labels Oct 27, 2017
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have this when Git wasn't an extension. Now that it is, this behaviour needs to go into API.

Would you like to continue the work and expose SourceControlInputBox.warningLength: number | undefined?

@jayjun
Copy link
Contributor Author

jayjun commented Oct 28, 2017

@joaomoreno Forgive my ignorance, still new to the codebase.

Is the API for exposing the setting from host to extension, extension to host, or both? As I argued above, I think users rarely need different length warnings for different SCM extensions.

@joaomoreno
Copy link
Member

It's our extension API. It's not a user setting, it would be an API exposed to an extension. Git might use 80, while Perforce doesn't have a limit.

@jayjun jayjun changed the title [WIP] Add character countdown to commit message input Add character countdown to commit message input Nov 11, 2017
@jayjun
Copy link
Contributor Author

jayjun commented Nov 11, 2017

@joaomoreno SourceControlInputBox.warningLength added.

I’ve hard-coded Git’s warning length for now because I’m unsure where it should go.

I would appreciate some guidance on localising strings as well. Do we have plural support?

@DuckyCrayfish
Copy link

Any update on this? It's been a month since there was any activity

@jayjun
Copy link
Contributor Author

jayjun commented Dec 8, 2017

@DuckyCrayfish It already works perfectly, and I occasionally rebase onto master while waiting for feedback.

If you want to see this merged, 👍 the topmost comment to upvote! That’s how Microsoft ranks what goes into the next release.

@DuckyCrayfish
Copy link

@jayjun Oh, wonderful, I got thrown off by the CI failure. Thanks for getting back to me, I'll give this a thumbs up.

@joaomoreno joaomoreno merged commit c407635 into microsoft:master Dec 12, 2017
@joaomoreno
Copy link
Member

Hi guys ✋ sorry for jumping in so late.

I've made a few changes:

  • The warning length should be per line, not for the whole value
  • There should be a way to control the behaviour, so I've introduced a scm.inputCounter setting with the following possible values: ['always', 'warn', 'none']. I've picked warn as the default: it only shows the counter once you are over the limit

Thanks! 🍻

@joaomoreno joaomoreno modified the milestones: Backlog, December 2017/January 2018 Dec 12, 2017
@jayjun jayjun deleted the commit-counter branch December 18, 2017 08:06
@Rylon
Copy link

Rylon commented Feb 8, 2018

Hi @joaomoreno did the scm.inputCounter setting make it into a release? I'm trying to set it in VS Code 1.20.0, but it doesn't seem to be recognised

@joaomoreno
Copy link
Member

@Rylon We had to change it to git.inputValidation at the last minute. Sorry for that! Let me know whether it works. 👍

@jlelong
Copy link
Contributor

jlelong commented Feb 9, 2018

Unfortunately, the configuration variable git.inputValidation is not documented on the release notes of 1.20. Moreover, the screenshot corresponds to git.inputValidation="always" whereas the default value is git.inputValidation="warn".

@joaomoreno
Copy link
Member

@jlelong Great catch! I've updated the setting name and described the screenshot's behaviour: microsoft/vscode-docs#1387 Thanks! 🍻

@borekb
Copy link

borekb commented Feb 9, 2018

I've also been struggling with this a bit, I was searching for "commit" and "length" in settings which didn't find anything because of this:

  // Controls when to show input validation.
  "git.inputValidation": "warn"

Maybe the comment could include a couple of useful keywords 😉

@joaomoreno
Copy link
Member

joaomoreno commented Feb 9, 2018

@borekb 38ab734 👍

@robsonsobral
Copy link

Please, @joaomoreno , any way to customize the characters warning limit?

@Rylon
Copy link

Rylon commented Feb 9, 2018

Thanks @joaomoreno "git.inputValidation": "off" has sorted it for me 👍

@joaomoreno
Copy link
Member

@robsonsobral Sure, would you like to submit a PR?

@borekb
Copy link

borekb commented Feb 9, 2018

any way to customize the characters warning limit?

There's an issue specifically for that: #18807.

@robsonsobral
Copy link

@joaomoreno , I'm not sure on how to proceed, but I gonna try.

@poke
Copy link

poke commented Feb 16, 2018

I really like that this feature has made it in but I would like it to actually follow the very common pattern of having a 50 character summary line, and then an empty line before allowing a 72 character body.

It would be really nice if that would work somehow or could be configured in a way, although I’m not sure if that would not conflict with that API setting mentioned above since that only applies globally, and would not allow for line-based validation.

@GabeDuarteM
Copy link

@poke I would definitely prefer the way you described to work to be implemented, but maybe doing what was described at #18807 (comment) could be a simpler solution, as it would attend all public.

It would be nicer though if you could add three settings to what we already have today, those being headerLength, bodyLength and footerLength (or some other name), that would validate the length of the header, body, and the footer of the commit, and if it is not compliant, show the warn that is implemented today

@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
git GIT issues scm General SCM compound issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet